Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add MessagePack codec #2371

Merged
merged 2 commits into from
Mar 3, 2024
Merged

Add MessagePack codec #2371

merged 2 commits into from
Mar 3, 2024

Conversation

johnbchron
Copy link
Contributor

This adds MessagePack codec support.

To quote MessagePack, "it's like JSON but fast and small". I was wanting to make a param payload that included a decently-sized byte array inline, and JSON is pretty inefficient at that, so I added MessagePack support.

The CI hasn't finished running on my machine but I decided to go ahead and submit it anyways because the change is so isolated.

@johnbchron johnbchron mentioned this pull request Feb 26, 2024
Merged
gbj
gbj reviewed Mar 1, 2024
View reviewed changes
Copy link
Collaborator

@gbj gbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it's great to have added implementations like this. See notes above about into_string vs into_bytes.

server_fn/src/codec/msgpack.rs Outdated Show resolved Hide resolved
server_fn/src/codec/msgpack.rs Outdated Show resolved Hide resolved
@benwis
Copy link
Contributor

benwis commented Mar 3, 2024

I think there is a deeper question here, of how many codecs we want to have available in leptos itself. I don't think this is an issue for MessagePack or this commit, but it's worth thinking about moving forward

@benwis
Copy link
Contributor

benwis commented Mar 3, 2024

Overall this looks good, thanks for contributing!

@benwis benwis merged commit 530087d into leptos-rs:main Mar 3, 2024
44 of 60 checks passed
@luckynumberke7in
Copy link
Contributor

I think there is a deeper question here, of how many codecs we want to have available in leptos itself. I don't think this is an issue for MessagePack or this commit, but it's worth thinking about moving forward

I haven't worked with this package before, so someone please feel free to correct me, but from skimming the package...honestly...should something like this be a flat replacement in most cases (1:1)?

These little improvements towards an optimal Leptos sound wonderful, especially for a nosql or graphql DB when possible.

@benwis
Copy link
Contributor

benwis commented Mar 5, 2024

I think there is a deeper question here, of how many codecs we want to have available in leptos itself. I don't think this is an issue for MessagePack or this commit, but it's worth thinking about moving forward

I haven't worked with this package before, so someone please feel free to correct me, but from skimming the package...honestly...should something like this be a flat replacement in most cases (1:1)?

These little improvements towards an optimal Leptos sound wonderful, especially for a nosql or graphql DB when possible.

A flat replacement? This isn't really for db access, this is for the server fn encoding between server and client.

@andrewdavidmackenzie andrewdavidmackenzie mentioned this pull request Mar 7, 2024
Merged
@johnbchron johnbchron deleted the add-msgpack-codec branch March 9, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbj gbj gbj left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@johnbchron @benwis @luckynumberke7in @gbj